Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Abstract sockets #470

Merged
merged 6 commits into from
Dec 21, 2021
Merged

Abstract sockets #470

merged 6 commits into from
Dec 21, 2021

Conversation

Mixaster995
Copy link
Contributor

@Mixaster995 Mixaster995 commented Dec 7, 2021 •
edited
Loading

Vladimir Popov added 3 commits December 7, 2021 16:19
@Mixaster995
Rework memif to use abstract sockets
8037a7a 
Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
@Mixaster995
Add tests for proxy
0f5bf22 
Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
@Mixaster995
Rework memif to new api, add WithExternalVPP option
e87be57 
Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
This was referenced Dec 7, 2021
Closed
Closed
Closed
Closed
Closed
@edwarnicke
Copy link
Member

edwarnicke commented Dec 9, 2021 •
edited
Loading

@Mixaster995 Have you tested this with cmd-forwarder-vpp ? My initial testing with it is failing (though need to check to see if I'm adapting to it correctly to be sure).

@Mixaster995
Copy link
Contributor Author

Mixaster995 commented Dec 13, 2021 •
edited
Loading

@edwarnicke yes, i tested it on equinix metal, everything worked fine. For correct testing we need to update and build images for cmd-forwarder-vpp, cmd-nsc-vpp, cmd-icmp-responder-vpp with sdk-vpp replaced to this. And also we need to use patches as in here networkservicemesh/deployments-k8s#3305

Note: Calico doesn't work locally on kind (probably because of insufficient resources)

@edwarnicke
Copy link
Member

@Mixaster995 Good to know its working on equinix metal. My point is: could you push your code that fixes up cmd-forwarder-vpp for this change so I can try running its unit tests to make sure they are working as expected?

edwarnicke
edwarnicke reviewed Dec 15, 2021
View reviewed changes
.github/workflows/ci.yaml
if: matrix.os != 'windows-latest'
run: sudo -E PATH="$PATH" bash -c "go test -race ./..."
- name: Test
if: matrix.os == 'windows-latest'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain why this change?

Copy link
Contributor Author

@Mixaster995 Mixaster995 Dec 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test https://github.com/Mixaster995/sdk-vpp/blob/abstract-sockets/pkg/tools/proxy/proxy_test.go#L44
failes without these lines
The reason is creation of a new network namespace throws operation not permitted on linux, so we have to use sudo, and this construction in ci passes correct envs to it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it :)

edwarnicke
edwarnicke reviewed Dec 15, 2021
View reviewed changes
pkg/networkservice/mechanisms/memif/common.go Outdated
)

// nolint:gochecknoinits
func init() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we doing this in an init() ?

Copy link
Contributor Author

@Mixaster995 Mixaster995 Dec 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

made some refactoring, removed init method

retested that with that changes everything works on kind and also works with calico on equinix metal

@Mixaster995
refactoring of init()
e6df795 
Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>
edwarnicke
edwarnicke reviewed Dec 16, 2021
View reviewed changes
pkg/networkservice/mechanisms/memif/common.go Outdated
netNS netns.NsHandle
netNSPath string
once sync.Once
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mixaster995 these should not be global variables, they should be attributes of the chain(s) that use them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@edwarnicke made refactoring to remove usage of global variables - please, take a look

@Mixaster995
refactored to remove global variables
3c68e61 
Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>
edwarnicke
edwarnicke reviewed Dec 17, 2021
View reviewed changes
pkg/networkservice/chains/forwarder/server.go Outdated
@@ -79,6 +79,7 @@ func NewServer(ctx context.Context, name string, authzServer networkservice.Netw
)
nsClient := registryclient.NewNetworkServiceRegistryClient(ctx, clientURL, registryclient.WithDialOptions(clientDialOptions...))

netNsInfo := memif.NewNetNSInfo()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mixaster995 Why are we doing this outside the memif.NewServer() ? If this can be safely done inside memif.NewServer it vastly simplifies the use of memif.NewServer...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@edwarnicke i was under impression that it was required for some logic, but was mistaken. Made some refactoring and tested all changes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mixaster995 Its all good. When reworking someone else's work, its sometimes hard to figure out if something that looks weird is essential or not.

@Mixaster995
refactoring for not using shared variables
78e713d 
Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>
@denis-tingaikin denis-tingaikin merged commit 0fd2c25 into networkservicemesh:main Dec 21, 2021
nsmbot pushed a commit to networkservicemesh/cmd-nsc-vpp that referenced this pull request Dec 21, 2021
Update go.mod and go.sum to latest version from networkservicemesh/sd…
522d1ed 
…k-vpp@main

PR link: networkservicemesh/sdk-vpp#470

Commit: 0fd2c25
Author: Авраменко Михаил
Date: 2021-12-21 15:07:28 +0700
Message:
  - Abstract sockets (#470)
* Rework memif to use abstract sockets

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

* Add tests for proxy

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

* Rework memif to new api, add WithExternalVPP option

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

* refactoring of init()

Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>

* refactored to remove global variables

Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>

* refactoring for not using shared variables

Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-nse-icmp-responder-vpp that referenced this pull request Dec 21, 2021
Update go.mod and go.sum to latest version from networkservicemesh/sd…
5202f7b 
…k-vpp@main

PR link: networkservicemesh/sdk-vpp#470

Commit: 0fd2c25
Author: Авраменко Михаил
Date: 2021-12-21 15:07:28 +0700
Message:
  - Abstract sockets (#470)
* Rework memif to use abstract sockets

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

* Add tests for proxy

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

* Rework memif to new api, add WithExternalVPP option

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

* refactoring of init()

Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>

* refactored to remove global variables

Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>

* refactoring for not using shared variables

Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-forwarder-vpp that referenced this pull request Dec 21, 2021
Update go.mod and go.sum to latest version from networkservicemesh/sd…
9e75786 
…k-vpp@main

PR link: networkservicemesh/sdk-vpp#470

Commit: 0fd2c25
Author: Авраменко Михаил
Date: 2021-12-21 15:07:28 +0700
Message:
  - Abstract sockets (#470)
* Rework memif to use abstract sockets

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

* Add tests for proxy

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

* Rework memif to new api, add WithExternalVPP option

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

* refactoring of init()

Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>

* refactored to remove global variables

Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>

* refactoring for not using shared variables

Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-nse-firewall-vpp that referenced this pull request Dec 21, 2021
Update go.mod and go.sum to latest version from networkservicemesh/sd…
983dc90 
…k-vpp@main

PR link: networkservicemesh/sdk-vpp#470

Commit: 0fd2c25
Author: Авраменко Михаил
Date: 2021-12-21 15:07:28 +0700
Message:
  - Abstract sockets (#470)
* Rework memif to use abstract sockets

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

* Add tests for proxy

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

* Rework memif to new api, add WithExternalVPP option

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

* refactoring of init()

Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>

* refactored to remove global variables

Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>

* refactoring for not using shared variables

Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-nse-vlan-vpp that referenced this pull request Dec 21, 2021
Update go.mod and go.sum to latest version from networkservicemesh/sd…
60c4a71 
…k-vpp@main

PR link: networkservicemesh/sdk-vpp#470

Commit: 0fd2c25
Author: Авраменко Михаил
Date: 2021-12-21 15:07:28 +0700
Message:
  - Abstract sockets (#470)
* Rework memif to use abstract sockets

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

* Add tests for proxy

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

* Rework memif to new api, add WithExternalVPP option

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

* refactoring of init()

Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>

* refactored to remove global variables

Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>

* refactoring for not using shared variables

Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
This was referenced Dec 21, 2021
Merged
Closed
Closed
Merged
@nsmbot nsmbot mentioned this pull request Dec 21, 2021
Closed
This was referenced Dec 27, 2021
Closed
Closed
@denis-tingaikin denis-tingaikin mentioned this pull request Mar 25, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@denis-tingaikin denis-tingaikin denis-tingaikin approved these changes

@edwarnicke edwarnicke edwarnicke approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Mixaster995 @edwarnicke @denis-tingaikin